fix(config): fix default regexes and references in docs #7

Groxx · 2021-09-05T21:55:36Z

Description

Fixes a likely bug in the default message regexes.

Motivation and Context

A regex like ^feat* will match all of these: https://regex101.com/r/esaUhE/1

feat
feature
fea
featttttttttttttttttt
fealty

Likely this was intended to act like ^feat.*, but as it was working correctly on "feat: add xyz" it must not need to match the whole string, so the trailing "any characters .*" is unnecessary.

In some of these, it may be desirable to add a \b on the end to match only whole words, not partial... but likely not all, as e.g. allowing "feat" to match "feature", and "fix" to match "fixes" is probably a good thing. I haven't done that here mostly because that seems like a lot more of a design choice than a bug :)

How Has This Been Tested?

Prior to changing the regex in integration_tests.rs, adding this:

diff --git a/git-cliff-core/tests/integration_test.rs b/git-cliff-core/tests/integration_test.rs
index d226469..6d4e8a2 100644
--- a/git-cliff-core/tests/integration_test.rs
+++ b/git-cliff-core/tests/integration_test.rs
@@ -56,6 +56,7 @@ fn generate_changelog() -> Result<()> {
                                Commit::new(String::from("abc124"), String::from("feat: add zyx")),
                                Commit::new(String::from("def789"), String::from("invalid commit")),
                                Commit::new(String::from("qwerty"), String::from("fix: fix abc")),
+                               Commit::new(String::from("qwerty"), String::from("final: invalid commit")),
                                Commit::new(
                                        String::from("hjkl12"),
                                        String::from("chore: do boring stuff"),

fails because the "final: invalid commit" is included in the output:

     Running tests/integration_test.rs (target/debug/deps/integration_test-464c48ac65477f2c)

running 1 test
test generate_changelog ... FAILED

failures:

---- generate_changelog stdout ----
thread 'generate_changelog' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
<"this is a changelog\n\n        ## Release v2.0.0\n        \n        ### fix bugs\n        \n        - fix abc\n        \n        ### shiny features\n        \n        - add xyz\n        - add zyx\n        \n        ## Release v1.0.0\n        \n        ### chore\n        \n        - do nothing\n        \n        ### feat\n        \n        - add cool features\n        \n        ### fix\n        \n        - fix stuff\n        - fix more stuff\n        eoc - end of changelog\n"
>"this is a changelog\n\n        ## Release v2.0.0\n        \n        ### fix bugs\n        \n        - fix abc\n        - invalid commit\n        \n        ### shiny features\n        \n        - add xyz\n        - add zyx\n        \n        ## Release v1.0.0\n        \n        ### chore\n        \n        - do nothing\n        \n        ### feat\n        \n        - add cool features\n        \n        ### fix\n        \n        - fix stuff\n        - fix more stuff\n        eoc - end of changelog\n"

', git-cliff-core/tests/integration_test.rs:114:

Using ^fix (as in this PR) correctly filters it out.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (no code change)
Refactor (refactoring production code)
Other

Checklist:

My code follows the code style of this project (after rustup default nightly, all pass)
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

Groxx · 2021-09-05T22:22:17Z

Should I amend my commit(s) to have the end commit message, and keep one / a branch to merge / will you squash and rewrite? generally I just copy from the PR description (minus process-checkmarks and similar), but I'm happy to rewrite to whatever you'd prefer.

codecov-commenter · 2021-09-06T14:49:22Z

Codecov Report

Merging #7 (0f4a389) into main (8fb18b7) will increase coverage by 0.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main       #7      +/-   ##
==========================================
+ Coverage   42.38%   42.72%   +0.34%     
==========================================
  Files          13       13              
  Lines         597      597              
  Branches      162      162              
==========================================
+ Hits          253      255       +2     
+ Misses        254      250       -4     
- Partials       90       92       +2

Impacted Files	Coverage Δ
git-cliff-core/src/repo.rs	`43.08% <0.00%> (+3.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fb18b7...0f4a389. Read the comment docs.

orhun · 2021-09-06T14:57:43Z

Hello! Thanks for taking time to submit this, I know something was odd with my regexes 😄

Likely this was intended to act like ^feat.*, but as it was working correctly on "feat: add xyz" it must not need to match the whole string, so the trailing "any characters .*" is unnecessary.

Makes sense.

In some of these, it may be desirable to add a \b on the end to match only whole words, not partial... but likely not all, as e.g. allowing "feat" to match "feature", and "fix" to match "fixes" is probably a good thing. I haven't done that here mostly because that seems like a lot more of a design choice than a bug :)

That's right 👍🏼

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)

arguably at least

I think that's fine. Matching only the type of the commit (e.g. feat) was the intended behavior anyways.

Checklist:

My code follows the code style of this project.

cargo fmt --all -- --check --verbose has multiple unrelated failures on main, so I'm not too concerned there

CI seems to be failing, can you run cargo fmt and push the changes?

edit/note: rustfmt.toml requires Rust nightly.

Should I amend my commit(s) to have the end commit message, and keep one / a branch to merge / will you squash and rewrite? generally I just copy from the PR description (minus process-checkmarks and similar), but I'm happy to rewrite to whatever you'd prefer.

I can squash the commits, no problem.

And I was wondering if we should update the regexes in the other templates as well. ({detailed,keepachangelog}.toml) What do you think?

Groxx · 2021-09-06T22:26:23Z

Checklist:

My code follows the code style of this project.

cargo fmt --all -- --check --verbose has multiple unrelated failures on main, so I'm not too concerned there

CI seems to be failing, can you run cargo fmt and push the changes?

edit/note: rustfmt.toml requires Rust nightly.

I'd thought cargo did that, but it looks like no - will do.

And I was wondering if we should update the regexes in the other templates as well. ({detailed,keepachangelog}.toml) What do you think?

Yep - I only searched for "feat*" or something, so I just missed them. I'll add them as well.
(... which are obviously matches. maybe the github-web-vscode ignored those files or something? weird.)

Groxx · 2021-09-06T22:38:16Z

Huh. cargo +nightly fmt --all behaves differently than rustup default nightly + cargo fmt --all.

I also realized that you may want to change the version regexes. Currently they're v[0-9]* which allows v on its own. v[0-9]+ (require at least one digit) might be what you want?

orhun · 2021-09-07T08:51:04Z

I also realized that you may want to change the version regexes. Currently they're v[0-9]* which allows v on its own. v[0-9]+ (require at least one digit) might be what you want?

They are glob patterns not regex so v[0-9]+ won't really work I think.

Other than that everything seems fine to me. Just one thing, I recently pushed b5df656 and the regexes in this commit (^doc*) should be updated as well. Can you do it or should I do it myself after merge? We can merge if you're not planning to make any further changes.

orhun · 2021-09-08T18:32:23Z

I think I'm going to merge this and update the other regexes afterwards. Thanks again!

Groxx requested a review from orhun as a code owner September 5, 2021 21:55

Groxx changed the title ~~Fix default regexes and references in docs~~ Fix: default regexes and references in docs Sep 5, 2021

Groxx and others added 4 commits September 6, 2021 15:42

Fix default regexes and references in docs

246dc5b

correction: I didn't realize the colon was necessary

0c38eb7

change more tomls, fmt

625de8a

rebase fixes

2c15cfe

Groxx force-pushed the xkcd-1171 branch from 0234ddb to 2c15cfe Compare September 6, 2021 22:45

orhun approved these changes Sep 8, 2021

View reviewed changes

orhun changed the title ~~Fix: default regexes and references in docs~~ fix(config): fix default regexes and references in docs Sep 8, 2021

orhun merged commit 8a18e4d into orhun:main Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(config): fix default regexes and references in docs #7

fix(config): fix default regexes and references in docs #7

Groxx commented Sep 5, 2021 •

edited

Loading

Groxx commented Sep 5, 2021

codecov-commenter commented Sep 6, 2021

orhun commented Sep 6, 2021 •

edited

Loading

Types of changes

Checklist:

Groxx commented Sep 6, 2021 •

edited

Loading

Checklist:

Groxx commented Sep 6, 2021 •

edited

Loading

orhun commented Sep 7, 2021

orhun commented Sep 8, 2021

fix(config): fix default regexes and references in docs #7

fix(config): fix default regexes and references in docs #7

Conversation

Groxx commented Sep 5, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Groxx commented Sep 5, 2021

codecov-commenter commented Sep 6, 2021

Codecov Report

orhun commented Sep 6, 2021 • edited Loading

Types of changes

Checklist:

Groxx commented Sep 6, 2021 • edited Loading

Checklist:

Groxx commented Sep 6, 2021 • edited Loading

orhun commented Sep 7, 2021

orhun commented Sep 8, 2021

Groxx commented Sep 5, 2021 •

edited

Loading

orhun commented Sep 6, 2021 •

edited

Loading

Groxx commented Sep 6, 2021 •

edited

Loading

Groxx commented Sep 6, 2021 •

edited

Loading